Skip to content

fix: review fixes for #630 (matrix accessor rewrite)#632

Merged
FabianHofmann merged 3 commits intoperf/matrix-accessor-rewritefrom
perf/matrix-accessor-rewrite-review
Mar 25, 2026
Merged

fix: review fixes for #630 (matrix accessor rewrite)#632
FabianHofmann merged 3 commits intoperf/matrix-accessor-rewritefrom
perf/matrix-accessor-rewrite-review

Conversation

@FabianHofmann
Copy link
Collaborator

@FabianHofmann FabianHofmann commented Mar 24, 2026

Changes proposed in this Pull Request

Review fixes for #630. Addresses some issues found during code review.

Fixed

  • Constraint.__repr__ passed CSR column positions instead of variable labels to print_single_expression, producing wrong output. Now maps back via vlabels[csr.indices[...]].
  • Constraints.set_blocks crashed on frozen Constraint — accessed ._data which only exists on MutableConstraint. Now handles both types. -> this is not important for now but will be in case we want to revive the pips interface
  • extracted _active_to_dataarray helper on Constraint
  • Constraints.reset_dual — simplified to c._dual = None.

Identified but Deferred

  • Constraint.__repr__ duplicates ConstraintBase.__repr__ rendering logic
  • Constraint called "immutable" but sanitize_* mutates — perhaps we should rename it to "frozen" ?
  • VariableLabelIndex.__init__ uses Any instead of Variables
  • Constraint.coords triggers silent Dataset reconstruction (no PerformanceWarning)
  • Constraints.to_matrix duplicates MatrixAccessor._build_cons stacking
  • MatrixAccessor.dual complex isinstance branching
  • Constraint.to_polars() return type Any -> pl.DataFrame
  • Missing PerformanceWarning emission tests
  • coords_to_dataset_vars/coords_from_dataset untested
  • Missing loc on frozen Constraint

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

- Fix __repr__ passing CSR positions instead of variable labels
- Fix set_blocks failing on frozen Constraint
- Extract _active_to_dataarray helper to reduce DRY violations
- Simplify reset_dual to direct mutation instead of reconstruction
- Add tests for freeze/mutable roundtrip, VariableLabelIndex,
  to_matrix_with_rhs, from_mutable mixed signs, repr correctness
…hs setters

- Store _sign as str (uniform, fast) or np.ndarray (mixed, per-row)
- Add rhs/lhs setters on Constraint via _refreeze_after pattern
- Invalidate _dual on mutation; update netcdf serialization for array signs
- Add tests for setters, mixed-sign freeze/roundtrip/sanitize/repr/netcdf
@FabianHofmann FabianHofmann force-pushed the perf/matrix-accessor-rewrite-review branch from c9b2abf to c8a4ddc Compare March 25, 2026 15:36
@FabianHofmann FabianHofmann merged commit 3b2a415 into perf/matrix-accessor-rewrite Mar 25, 2026
2 checks passed
@FabianHofmann FabianHofmann deleted the perf/matrix-accessor-rewrite-review branch March 25, 2026 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant